Cosmos: fix #2616 by using RawValue, and fix query rewriting #2617
+26
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
analogrelay
force-pushed
the
ashleyst/2616-fix-up-query-engine
branch
from
e237068 to
dfe2868
Compare
analogrelay
requested review from
Pilchie,
kirankumarkolli,
tvaron3,
FabianMeiswinkel and
kundadebdatta
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #2616 by addressing the query engine’s contract with RawValue and ensuring that the rewritten query from the pipeline is used by the SDK executor. Key changes include updating the return type to Box for query items, modifying the executor to properly set and use the base query request, and adjusting the Cargo.toml feature flag for the preview query engine.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/cosmos/azure_data_cosmos/tests/framework/query_engine.rs | Updated pop_item return type to use RawValue. |
| sdk/cosmos/azure_data_cosmos/src/query/mod.rs | Exposed the query text field to accommodate rewriting. |
| sdk/cosmos/azure_data_cosmos/src/query/executor.rs | Revised executor initialization and query rewriting to use the pipeline’s authoritative query. |
| sdk/cosmos/azure_data_cosmos/src/query/engine.rs | Modified PipelineResult to use Box for items. |
| sdk/cosmos/azure_data_cosmos/Cargo.toml | Adjusted the preview_query_engine feature to enable serde_json/raw_value support. |
Comments suppressed due to low confidence (2)
sdk/cosmos/azure_data_cosmos/src/query/executor.rs:74
- [nitpick] Consider using a more descriptive expect message that provides additional context about the base_request's usage to aid debugging.
self.base_request.as_ref().expect("base_request should be set when pipeline is set")
sdk/cosmos/azure_data_cosmos/src/query/executor.rs:110
- [nitpick] Consider replacing the generic expect message "we just set it" with a more informative message to better explain the assumption behind pipeline initialization.
self.pipeline.as_mut().expect("we just set it")
heaths
approved these changes
May 17, 2025
heaths
approved these changes
May 20, 2025
Pilchie
approved these changes
May 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2616
It's a tale as old as time. Abstraction is written, then implemented, and found lacking somewhere ;). This PR has a few small fix-ups for the query engine abstraction that I found while integrating the query engine. Once these were fixed, it worked perfectly though! 🎉
Here are the changes:
The query engine works with
serde_json::value::RawValue, which represents "unparsed" JSON. It's just a wrapper aroundstr, and I had hoped it would be easy to consume it and convert it in to aVec<u8>, so that's what I had the QueryPipeline abstraction expect as a result from the pipeline. Alas, it's not really possible to do that. You can access the underlying bytes viaRawValue::get()but that returns&strand there's no way to "move" the bytes out of theRawValue. But that's OK, we can just have the pipeline send backBox<RawValue>values directly, since the pipeline is always working with JSON anyway. So, that's what I did. (This is #2616, which I filed when I thought I could work around it and fix the SDK later, but I couldn't do that with the below issue so I decided to just fix them both here first).There was also a bug in how the query "executor" worked. I built the base query request as soon as the pipeline was created, using the query the user passed in. But the gateway may rewrite the query. The query engine handles this and exposes the "authoritative" query out of
QueryPipeline::query(), but the SDK's executor ignored that and used the original query. A pretty simple fix.